Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/urr #69

Merged
merged 26 commits into from
Mar 28, 2024
Merged

Feat/urr #69

merged 26 commits into from
Mar 28, 2024

Conversation

ianchen0119
Copy link
Contributor

  • support to do the long-term test by using go test.
  • support to handle Session Report & URR-related IEs.

@ianchen0119
Copy link
Contributor Author

ianchen0119 commented Mar 14, 2024

Hi @gab-arrobo & @thakurajayL

Could you please help to review the PR?
Thanks.

Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few issues that need to be resolved such as apply gofmt to pkg/pfcpsim/session/urr_ies.go, add missing license/copyright headers to a couple files

fuzz/ie_fuzz_test.go Show resolved Hide resolved
internal/pfcpsim/export/export.go Outdated Show resolved Hide resolved
.golangci.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@ianchen0119
Copy link
Contributor Author

Hi @gab-arrobo

Do I need to do any enhancements for this PR?
If yes, please let me know when you feel free.

Many thanks.

@gab-arrobo
Copy link
Contributor

Hi @gab-arrobo

Do I need to do any enhancements for this PR? If yes, please let me know when you feel free.

Many thanks.

Hi @ianchen0119, I will try to review your PR in the coming days and will get back to you

Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianchen0119, how did you test the changes/PR?

Makefile Show resolved Hide resolved
@gab-arrobo
Copy link
Contributor

@ianchen0119, I just enabled some additional linters. Please rebase your PR. Thank you!

@ianchen0119
Copy link
Contributor Author

@ianchen0119, how did you test the changes/PR?

Hi @gab-arrobo

After the UPF gets started, the fuzzing can be enabled.
It will connect to the UPF and try to send the malformed PFCP message to explore the potential security issue.

We can use the command below to trigger the fuzzing, for example:

go test -fuzz=Fuzz -p 1 -parallel 1 -fuzztime 15m ./fuzz/...

Furthermore, go-upf PR #33 is to fix the bug, reported by a community user, and it can be reproduced by the fuzzing.

@andy89923
Copy link
Contributor

Hello,

I push a commit to let PFCP accept the loopback interface.
Also, this PR was functional in my environment.

截圖 2024-03-27 下午2 37 38

I run the UPF in host (10.10.0169), and run PFCPSIM on the same machine. It can create sessions, delete sessions, and also receive reports from UPF.

@ianchen0119
Copy link
Contributor Author

Hi @gab-arrobo
Could you please help to approve the CI workflow?

Thank you.

@gab-arrobo
Copy link
Contributor

Hello,

I push a commit to let PFCP accept the loopback interface. Also, this PR was functional in my environment.

截圖 2024-03-27 下午2 37 38

I run the UPF in host (10.10.0169), and run PFCPSIM on the same machine. It can create sessions, delete sessions, and also receive reports from UPF.

Thanks for your input/update. I am going to try it.

Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good to me.

Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
fuzz/ie_fuzz_test.go Outdated Show resolved Hide resolved
fuzz/ie_fuzz_test.go Outdated Show resolved Hide resolved
@andy89923
Copy link
Contributor

Hello,

Thanks for the suggestion! @gab-arrobo

These were what I did in the latest commit:

  • Added the args to fuzz test for user-friendly run testing
  • Fix typo in code comments
  • Update README.md about how to use fuzzy test and the expected/unexpected result output

Copy link
Contributor

@sureshmarikkannu sureshmarikkannu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get a chance to test this PR (i haven't tried pfcpsim yet) and i walkthrough the changes from coding perspective and it looks good to me.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 (Run a test using UPF w/ OnRamp)

@gab-arrobo gab-arrobo merged commit 2f3579b into omec-project:main Mar 28, 2024
7 checks passed
@ianchen0119 ianchen0119 deleted the feat/urr branch March 28, 2024 16:16
}
}
}

func (c *PFCPClient) ConnectN4(remoteAddr string) error {
func (c *PFCPClient) ConnectN4(ctx context.Context, remoteAddr string) error {
Copy link
Contributor

@gab-arrobo gab-arrobo Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianchen0119, why do you need to add context to this function? Does not it already have context when the NewPFCPClient is created/called (https://github.com/omec-project/pfcpsim/blob/main/pkg/pfcpsim/pfcpsim.go#L112)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's used for receiveFromN4().

go c.receiveFromN4(ctx)

And receiveFromN4():

func (c *PFCPClient) receiveFromN4(ctx context.Context) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gab-arrobo @andy89923

I Just created the PR to fix it: #74

README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants